Skip to content

feat: Add Annotated Tag Push Support#1139

Open
fabiovincenzi wants to merge 9 commits intofinos:mainfrom
fabiovincenzi:push-tags
Open

feat: Add Annotated Tag Push Support#1139
fabiovincenzi wants to merge 9 commits intofinos:mainfrom
fabiovincenzi:push-tags

Conversation

@fabiovincenzi
Copy link
Contributor

@fabiovincenzi fabiovincenzi commented Aug 4, 2025

Description:
This PR introduces end-to-end support for annotated tag pushes

  • Split the push pipeline into separate branchPushChain and tagPushChain.
  • Update getChain() to select the correct chain based on action.type and presence of action.tag.
  • Render a dedicated “Tag Details” table when viewing a tag push instead of the commit diff.

Fixes #986

@netlify
Copy link

netlify bot commented Aug 4, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 8dec82c
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/69b138e358128e00081f774b

@codecov
Copy link

codecov bot commented Aug 6, 2025

Codecov Report

❌ Patch coverage is 97.60000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.15%. Comparing base (90df884) to head (8dec82c).

Files with missing lines Patch % Lines
src/proxy/processors/push-action/parsePush.ts 95.45% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1139      +/-   ##
==========================================
+ Coverage   90.01%   90.15%   +0.13%     
==========================================
  Files          67       67              
  Lines        4769     4865      +96     
  Branches      885      913      +28     
==========================================
+ Hits         4293     4386      +93     
- Misses        458      461       +3     
  Partials       18       18              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

} else if (action.tagData?.length) {
action.user = action.tagData.at(-1)!.tagger;
} else {
throw new Error('No commit or tag data parsed from packfile');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the commitData check here is causing this test to fail:

it.only('should return empty commitData on empty branch push', async () => {
  const emptyPackBuffer = createEmptyPackBuffer();

  const newCommit = 'b'.repeat(40);
  const ref = 'refs/heads/feature/emptybranch';
  const packetLine = `${EMPTY_COMMIT_HASH} ${newCommit} ${ref}\0capabilities\n`;

  req.body = Buffer.concat([createPacketLineBuffer([packetLine]), emptyPackBuffer]);

  const result = await exec(req, action);

  expect(result).to.equal(action);

  const step = action.steps.find(s => s.stepName === 'parsePackFile');
  expect(step).to.exist;
  expect(step.error).to.be.false; // <-- Fails here
  expect(action.branch).to.equal(ref);
  expect(action.setCommit.calledOnceWith(EMPTY_COMMIT_HASH, newCommit)).to.be.true;

  expect(action.commitData).to.be.an('array').with.lengthOf(0);
});

Ideally, we want to allow empty commitData so that we can interpret it later: It might be an empty branch push, a tag push or something else entirely.

Perhaps we could move this check into the another action? We could rename the checkEmptyBranch action to handleEmptyCommitData and do all the relevant checks there.

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core flow seems to work correctly! Just a few edge cases and an integration issue with the checkEmptyBranch that might be worth refactoring.

Also, we will be merging #973 soon which will likely cause a few conflicts here... Might want an extra pair of eyes to make sure everything is good to merge.

@fabiovincenzi
Copy link
Contributor Author

Updated with the latest changes of main and manually tested also with gitlab

action = await proc.pre.parseAction(req);
// 2) Parse the push payload first to detect tags/branches
if (action.type === RequestType.PUSH) {
action = await proc.push.parsePush(req, action);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I misunderstood the flow here, but isn't parsePush executing twice when pushing branches? Seems it executes here, and once again as part of the branchPushChain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, it seems the push flow is working as expected - just not sure if this is running twice or if the parsePush prevents the double execution somehow.

Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I tested the tags and regular push flow and both work as expected with the new updates 👍🏼

Just a comment on the parsePush which I'm not sure if it's executing once or twice on pushes.

@jescalada jescalada requested a review from a team August 27, 2025 10:10
@jescalada
Copy link
Contributor

As we're modifying the proxy logic, another pair of eyes here would be super helpful! 👀 @finos/git-proxy-maintainers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for pushing tags git push --tags Add support for pushing tags git push --tags

2 participants